Skip to content

CI: hermetic spawns in cli/covgap tests (#823) - #1441

Merged
Mikola Lysenko (mikolalysenko) merged 3 commits into
mainfrom
agent/fix-ci-hermetic-cli-covgap
Oct 12, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 3 commits into
mainfrom
agent/fix-ci-hermetic-cli-covgap

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

LLM Description written by Claude Code:claude-opus-5-5

Refs #823 (slice 4 of the hermetic-spawn migration; the issue stays open for the remaining files).

What was wrong

20 CLI test files (cli/, cli_*, coverage_fix_*, covgap_*) spawned socket-patch with a bare Command::new and their own hand-rolled SOCKET_* scrub. The copies disagreed. Several removed only the global flag vars, so SOCKET_API_TOKEN, SOCKET_API_URL or a per-command var like SOCKET_SKIP_ROLLBACK from a developer shell still reached the child. Two plain list runs in cli_parse_list scrubbed nothing, and none forced the update notifier off.

There was also a race in the shared builder. hermetic::command removed only the SOCKET_* names present when it swept the parent env. Test binaries that set flag-bound vars in-process (apply_env_toggles, save/restore helpers) could leak one set between the sweep and the spawn, and each suite patched that with its own removal list. Those lists had already drifted: 4 vars in one place, 2 in another, the full list in a third.

Change (test-only)

  • All 20 files spawn through tests/common/hermetic.rs and leave PENDING_RAW_SPAWNS (126 → 106), so the ratchet keeps them hermetic. Deliberate opt-ins stay as caller env: the config-layer suites still set SOCKET_NO_CONFIG=0, and the telemetry suite still re-enables telemetry.
  • hermetic::command now removes every flag-bound var (GLOBAL_ARG_ENV_VARS ∪ LOCAL_ARG_ENV_VARS) by name, which closes the race once and deletes the per-file lists. SOCKET_CWD joins the hostile seeds (the telemetry suite used to seed it locally). New contract test: command_drops_flag_bound_vars_set_after_it_is_built.
  • No workflow change. A lean PR run keeps the same job count (~28 Linux jobs) and job-minutes.

Evidence

  • Hostile shell (SOCKET_DRY_RUN=true SOCKET_JSON=true SOCKET_API_TOKEN=… SOCKET_API_URL=http://127.0.0.1:1 SOCKET_PROXY_URL=… SOCKET_SKIP_ROLLBACK=true SOCKET_ALL_RELEASES=true SOCKET_ORG_SLUG=… SOCKET_NO_CONFIG=0) over the 19 affected targets:
    • main: 11 failures. 6 spawn tests in cli_parse_list (*_via_binary), covgap_commands_vendor::npm_only_human_vendor_does_not_read_composer_lock, plus 4 noted below.
    • this branch: the 7 spawn tests pass. What remains is cli_parse_list::defaults_match_contract (an in-process clap parse, not a spawn; out of scope here) and the 3 root-only failures below.
  • command_drops_flag_bound_vars_set_after_it_is_built fails against main's hermetic.rs (SOCKET_OFFLINE set after build leaked) and passes with this change. spawn_env_hygiene: 15/15.
  • cargo test --workspace --all-features --no-fail-fast, 249 targets: everything passes except 13 tests that need a non-root user to make a file unwritable (*_write_failure_*, *unremovable*, relax_loop_must_not_traverse_symlinked_root) and e2e_bun_lockb::binary_shared_bundled_record_hosted_pin_is_managed. My container runs as root. Each of these fails the same way with main's hermetic.rs and test files, so none are from this PR. CI runs as non-root.
  • cargo fmt --all -- --check and cargo clippy --locked --workspace --all-features -- -D warnings: clean.

/code-review high: findings not fixed here

Follow-ups for #823: the 106 remaining PENDING_RAW_SPAWNS files (mostly e2e_*, in_process_*, vex_*), the PTY spawns, and the in-process parse in cli_parse_list.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CQp9sCPnUPTSFrEGsvTQmQ


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
Twenty CLI test files spawned socket-patch with a bare Command::new and
a hand-rolled SOCKET_* scrub. The copies disagreed: several only removed
the global flag vars, so SOCKET_API_TOKEN, SOCKET_API_URL or a
per-command var like SOCKET_SKIP_ROLLBACK from a developer shell still
reached the child, cli_parse_list's two plain list runs scrubbed
nothing, and none forced the update notifier off.

All 20 now spawn through the shared hermetic builder in
tests/common/hermetic.rs and leave PENDING_RAW_SPAWNS, so the ratchet
keeps them hermetic. Deliberate opt-ins stay as caller env: the config
layer suites still set SOCKET_NO_CONFIG=0 and the telemetry suite still
re-enables telemetry. Suites whose in-process tests mutate SOCKET_* keep
their unconditional removals for the spawn race.

Refs #823

Assisted-by: Claude Code:claude-opus-5-5
Some test binaries set flag-bound SOCKET_* vars in their own process
(apply_env_toggles, env save/restore helpers). The hermetic builder only
removed names present when it swept the parent environment, so a var
set by a parallel test before the spawn still reached the child, and
suites patched that with their own, already diverging, removal lists.

hermetic::command now removes every flag-bound var by name, so the
per-file lists go away, and SOCKET_CWD joins the hostile seeds. A new
contract test sets vars after the builder runs and checks none leak.

Refs #823

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 12, 2026 18:46
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 6d44a29. Configure here.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 12, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review (burn-down agent)

  • Head: 6d44a29a
  • CI: 86/86 settled on the current head (36 success, 50 path-skipped); no main-wide failures.
  • Mergeable against main, no CHANGELOG.md change.
  • Bugbot: reviewed 6d44a29, no findings; no open review threads.
  • Reviewer focus: the shared tests/common/hermetic.rs helper that the CLI/covgap tests now use to spawn the binary with a scrubbed environment.

Generated by Claude Code

Merged via the queue into main with commit 6ad6add Oct 12, 2026
86 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-ci-hermetic-cli-covgap branch October 12, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants